Sanchaga/pr 38 reviewbottest - #39

Issue: make_request action registered without read_only=False despite supporting mutating HTTP methods The decorator @app.make_request() is called with no arguments. SDK App.action() defaults to read_only=True. The action explicitly supports POST, PATCH, PUT, and DELETE via params.http_method, which are mutating operations.

Code reference: @app.make_request() decorator

Current line: def make_request(

Impact: SOAR marks the action as read-only in the generated manifest, preventing it from being used in write-enabled playbook contexts and misrepresenting its capability to users.

How to fix: Change the decorator to @app.make_request(read_only=False) so the generated metadata correctly reflects that this action can perform mutating API calls.

Issue: resolve_auth only supports personal_access_token, silently breaking username/password and OAuth client credentials src/client.py lines 54-60: only checks asset.personal_access_token; raises ActionFailure otherwise. The Asset model retains client_id, client_secret, username, and password fields. The legacy connector supported all three auth paths.

Code reference: resolve_auth function

Current line: 1. personal_access_token (PAT) → Authorization: Bearer

Impact: Existing SOAR assets configured with username/password or OAuth client credentials will fail immediately after upgrade with a misleading error. This is a silent backward-compatibility regression for any deployed non-PAT asset.

How to fix: Either add username/password (Basic auth) and client_id/client_secret (token-exchange or Basic auth) branches to resolve_auth matching the legacy connector's priority order, or explicitly document in release notes that only PAT is now supported and remove the unused credential fields from the Asset model.

Issue: Required non-optional fields in output models will fail Pydantic validation on sparse GitHub API responses Fields such as milestone, closed_by, assignee on GetIssueOutput and MilestoneOutput.creator: CreatorOutput are typed as required non-Optional. The GitHub Issues API documents these as nullable. A strict Pydantic model_validate() call on a real response where these are null (e.g. open issue with no milestone, issue created by deleted account) will raise ValidationError and fail the action.

Code reference: GetIssueOutput, MilestoneOutput, ClosedByOutput, AssigneeOutput, CreatorOutput

Current line: def get_issue(params: GetIssueParams, soar: SOARClient, asset: Asset) -> GetIssueOutput:

Impact: Any playbook calling get issue, create issue, update issue, or list issues on an open or unassigned issue will get a runtime validation failure instead of action data, breaking existing playbooks silently after the SDK migration.

How to fix: Annotate nullable nested objects as Optional: milestone: MilestoneOutput | None = None, closed_by: ClosedByOutput | None = None, assignee: AssigneeOutput | None = None, creator: CreatorOutput | None = None. Also audit URL template fields on user sub-models (e.g. avatar_url, events_url, followers_url, login, site_admin) and add | None where the GitHub API can return null.

Issue: remove_member returns status='success' when user is not a team member After exhausting both the active-members list and the pending-invitations list without finding the user, the function calls soar.set_message(GITHUB_USER_NOT_TEAM_MEMBER_MSG...) and then returns RemoveMemberOutput(status='success'). No removal occurred, but the action reports success.

Code reference: remove_member function, final return statement

Current line: soar.set_message(

Impact: A playbook checking action_result.status == 'success' will incorrectly conclude the user was removed. The correct behavior for a no-op non-removal is to fail the action so the playbook can branch appropriately.

How to fix: Raise an exception or use the SDK's failure mechanism instead of returning RemoveMemberOutput(status='success') when the user is not found in either the active-members or pending-invitations list.

Issue: ListCommentsSummary is defined but not passed to @app.action(), so it will not appear in generated metadata ListCommentsSummary is defined and soar.set_summary(ListCommentsSummary(...)) is called at runtime, but the @app.action() decorator at line 148 has no summary_cls= argument. The same pattern is present for list_users (ListUsersSummary) and list_issues (ListIssuesSummary).

Code reference: list_comments @app.action() decorator; ListCommentsSummary class

Current line: def list_comments(

Impact: Without the summary class wired into the action decorator, the generated app JSON omits the summary output path (e.g. action_result.summary.total_comments).

How to fix: Add summary_cls=ListCommentsSummary to the @app.action() decorator for list_comments, and apply the same fix to list_users and list_issues.